Skip to content

Conversation

@petrhosek
Copy link
Member

This addresses the issue uncovered by #115361. Previously, we weren't building benchmarks in many cases due to the following block:

if (WIN32 OR MINGW OR ANDROID OR ${CMAKE_SYSTEM_NAME} MATCHES "AIX"
OR NOT LIBCXX_ENABLE_LOCALIZATION
OR NOT LIBCXX_ENABLE_THREADS
OR NOT LIBCXX_ENABLE_FILESYSTEM
OR NOT LIBCXX_ENABLE_RANDOM_DEVICE
OR NOT LIBCXX_ENABLE_EXCEPTIONS
OR NOT LIBCXX_ENABLE_RTTI)
set(_include_benchmarks OFF)
else()
set(_include_benchmarks ON)
endif()

We need to passthrough the necessary variables into the benchmarks subbuild and use correct syntax.

This addresses the issue uncovered by llvm#115361. Previously, we weren't
building benchmarks in many cases due to the following block:

https://github.com/llvm/llvm-project/blob/e58949632e91477af58d983f3b66369e6a2c8233/libcxx/CMakeLists.txt#L162-L172

We need to passthrough the necessary variables into the benchmarks
subbuild and use correct syntax.
@petrhosek petrhosek requested a review from ldionne November 18, 2024 15:57
@petrhosek petrhosek requested a review from a team as a code owner November 18, 2024 15:57
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 18, 2024

@llvm/pr-subscribers-libcxx

Author: Petr Hosek (petrhosek)

Changes

This addresses the issue uncovered by #115361. Previously, we weren't building benchmarks in many cases due to the following block:

if (WIN32 OR MINGW OR ANDROID OR ${CMAKE_SYSTEM_NAME} MATCHES "AIX"
OR NOT LIBCXX_ENABLE_LOCALIZATION
OR NOT LIBCXX_ENABLE_THREADS
OR NOT LIBCXX_ENABLE_FILESYSTEM
OR NOT LIBCXX_ENABLE_RANDOM_DEVICE
OR NOT LIBCXX_ENABLE_EXCEPTIONS
OR NOT LIBCXX_ENABLE_RTTI)
set(_include_benchmarks OFF)
else()
set(_include_benchmarks ON)
endif()

We need to passthrough the necessary variables into the benchmarks subbuild and use correct syntax.


Full diff: https://github.com/llvm/llvm-project/pull/116644.diff

1 Files Affected:

  • (modified) libcxx/test/benchmarks/CMakeLists.txt (+4-3)
diff --git a/libcxx/test/benchmarks/CMakeLists.txt b/libcxx/test/benchmarks/CMakeLists.txt
index b5a4aae82c06ab..b0fe600623d964 100644
--- a/libcxx/test/benchmarks/CMakeLists.txt
+++ b/libcxx/test/benchmarks/CMakeLists.txt
@@ -35,13 +35,14 @@ ExternalProject_Add(google-benchmark
         SOURCE_DIR ${LLVM_THIRD_PARTY_DIR}/benchmark
         INSTALL_DIR ${CMAKE_CURRENT_BINARY_DIR}/google-benchmark
         CMAKE_CACHE_ARGS
-          -DCMAKE_C_COMPILER:STRING=${CMAKE_C_COMPILER}
-          -DCMAKE_CXX_COMPILER:STRING=${CMAKE_CXX_COMPILER}
+          -DCMAKE_C_COMPILER:FILEPATH=${CMAKE_C_COMPILER}
+          -DCMAKE_CXX_COMPILER:FILEPATH=${CMAKE_CXX_COMPILER}
+          -DCMAKE_MAKE_PROGRAM:FILEPATH=${CMAKE_MAKE_PROGRAM}
           -DCMAKE_BUILD_TYPE:STRING=RELEASE
           -DCMAKE_INSTALL_PREFIX:PATH=<INSTALL_DIR>
           -DCMAKE_CXX_FLAGS:STRING=${BENCHMARK_COMPILE_FLAGS}
           -DBENCHMARK_USE_LIBCXX:BOOL=ON
           -DBENCHMARK_ENABLE_TESTING:BOOL=OFF
-          -DBENCHMARK_CXX_LIBRARIES:STRING="${BENCHMARK_CXX_LIBRARIES}")
+          -DBENCHMARK_CXX_LIBRARIES:STRING=${BENCHMARK_CXX_LIBRARIES})
 
 add_dependencies(cxx-test-depends google-benchmark)

@PiJoules
Copy link
Contributor

ping is this ready to land?

@petrhosek
Copy link
Member Author

ping is this ready to land?

I need an approval from one of the libc++ reviewers.

@petrhosek
Copy link
Member Author

I'm going to land this change because our builders have been broken since #115361 landed and I'm not getting any response from @ldionne.

@petrhosek petrhosek merged commit 012dd8b into llvm:main Nov 19, 2024
63 of 64 checks passed
@ldionne
Copy link
Member

ldionne commented Nov 19, 2024

This LGTM. Sorry for the delay, I am at a C++ Committee Meeting this week and things are much more hectic than usual over here (stuff rushing for the C++26 deadline), so I haven't had time to do any code review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants